Skip to content

Count downloads for tag archives #25606

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

Count downloads for tag archives #25606

wants to merge 26 commits into from

Conversation

JakobDev
Copy link
Contributor

@JakobDev JakobDev commented Jun 30, 2023 •
edited
Loading

We currently count the downloads for Release Attachments, which is useful, but not for source archives. This PR changed this. Now downloads for source archives (tags only) are also counted!

grafik

TODO:

  • Reset download count, when tag is deleted (I don't know how to do this, so a help would be appreciated) (Done)
  • Add Migration

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 30, 2023
KN4CK3R
KN4CK3R reviewed Jun 30, 2023
View reviewed changes
@JakobDev
Copy link
Contributor Author

This should now be ready to review now. Only think left is Migration, but i will at this at the very least.

@JakobDev JakobDev requested a review from KN4CK3R July 24, 2023 14:09
@JakobDev
Copy link
Contributor Author

Does nobody want to review this PR?

@JakobDev
Copy link
Contributor Author

Bump

@lunny lunny added the type/enhancement An improvement of existing functionality label Oct 1, 2023
@JakobDev
Copy link
Contributor Author

Bump

lunny
lunny reviewed Oct 30, 2023
View reviewed changes
@JakobDev
Copy link
Contributor Author

JakobDev commented Dec 1, 2023

Is nobody interested in this?

@github-actions github-actions bot added the modifies/api This PR adds API routes or modifies them label Dec 5, 2023
@techknowlogick
Copy link
Member

Thanks @JakobDev, sorry this PR fell behind. I'll try to push a migration to this branch later today to complete the TODO list.

@techknowlogick
Copy link
Member

ack, didn't get around to the migration, but was able to resolve the linting issues that the CI was hanging on.

@JakobDev JakobDev requested a review from lunny February 28, 2024 09:15
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files labels Mar 18, 2024
lunny
lunny reviewed Mar 19, 2024
View reviewed changes
@JakobDev
Copy link
Contributor Author

I have now rewritten the Code to use the Release ID instead of the Tag as suggested by @lunny. I also added a migration and some improvements. I hope this will be merged soon.

lunny
lunny reviewed Mar 19, 2024
View reviewed changes
models/repo/archive_download_count.go Outdated
release, err := GetRelease(ctx, repoID, tagName)
if err != nil {
if IsErrReleaseNotExist(err) {
return new(api.TagArchiveDownloadCount), nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just return NotExist?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just to prevent a error in case this is somehow called with a wrong tag

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe returning the 404 error is better because the tag doesn't exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning the error causes problem when a tag exists in git but not in the db.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning the error causes problem when a tag exists in git but not in the db.

That's a problem should be fixed by other PRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So should I ignore Not Exists error then? I don't know if this can happen in the real world, but at least the tests are failing.

lunny
lunny reviewed Mar 19, 2024
View reviewed changes
techknowlogick
techknowlogick approved these changes Apr 2, 2024
View reviewed changes
Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lunny lunny lunny left review comments

@techknowlogick techknowlogick techknowlogick approved these changes

@KN4CK3R KN4CK3R Awaiting requested review from KN4CK3R

Assignees
No one assigned
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/migrations modifies/templates This PR modifies the template files type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@JakobDev @techknowlogick @lunny @KN4CK3R @GiteaBot